-
Notifications
You must be signed in to change notification settings - Fork 90
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MINIFICPP-1022 - Refactored third party build system #661
Conversation
Please read https://github.com/bakaid/nifi-minifi-cpp/blob/MINIFICPP-1022/ThirdParties.md before reviewing other parts of this PR. I still have to run tests to make sure I didn't break anything, including JNI and python. |
964e560
to
2c08b63
Compare
I like the idea of this PR for potentially 0.8 or 1.x, but I haven't resolved that in my head. I think the bigger thing is that perhaps we could move the third parties readme to a contribution folder and integrate it as a link in the contrib guide. Is there a way we could automate checks to for this? |
README.md
Outdated
@@ -530,6 +530,10 @@ On Windows it is suggested that MSI be used for installation. | |||
|
|||
Please see [Extensions.md](Extensions.md) on how to build and run conditionally built dependencies and extensions. | |||
|
|||
### Third parties | |||
|
|||
Please see [ThirdParties.md](ThirdParties.md) on how MiNiFi builds and uses third party libraries and how you can add new ones. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems more developer focused, perhaps this could be split into the contrib guide?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that's fair, moved the reference to CONTRIB.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Far from being done, some minor comments.
``` | ||
add_library(foo INTERFACE) | ||
target_include_directories(foo INTERFACE "${CMAKE_CURRENT_SOURCE_DIR}/thirdparty/libfoo-1.0.0") | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nicely written, cool doc, I like it!
2c08b63
to
a6d10aa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Went through the doc, looks good overall, some minor comments added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finished reviewing cmake changes, they look good!
Added some minor comments, but no red flags were spotted.
# specific language governing permissions and limitations | ||
# under the License. | ||
|
||
function(target_wholearchive_library TARGET ITEM) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May god bless you for removing all the copy-paste occurrences of this! 🙏
aefdc79
to
1d7c508
Compare
@arpadboda Addressed your review comments, I will start a final round of verification and self-review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM and seems to work for me on Debian, Win and Mac.
@arpadboda macOS failure on Travis is an unrelated timeout at the very end, I've finished my verification, I'll merge soon. |
Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
For all changes:
Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
Has your PR been rebased against the latest commit within the target branch (typically master)?
Is your initial contribution a single, squashed commit?
For code changes:
For documentation related changes:
Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.